fix(client-runtime): tolerate transient server stalls in connection heartbeat#3883
fix(client-runtime): tolerate transient server stalls in connection heartbeat#3883ramifara wants to merge 2 commits into
Conversation
…eartbeat The connection rewrite in pingdotgg#2978 removed protocol-level socket retry, so a single missed heartbeat pong now escalates to a full session teardown and re-setup. With the effect default heartbeat (ping every 5s, pong deadline 5s), any server event-loop stall or latency spike past 5 seconds drops the connection, which users experience as constant disconnect/reconnect popups. Relax the heartbeat to a 10s ping interval with a 20s pong deadline: pings stay frequent enough to hold NAT/firewall state, while the server gets room to stall under load before the link is declared dead. Genuine disconnects are still detected instantly via socket close events. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce unnecessary client session teardowns/reconnect loops by relaxing the WebSocket RPC heartbeat timings in @t3tools/client-runtime, so transient server stalls (e.g., under load or tunneled links) don’t get treated as hard disconnects and trigger the blocking “connecting…” UX.
Changes:
- Adds
pingInterval: "10 seconds"andpingTimeout: "20 seconds"options to theRpcClient.makeProtocolSocketcall in the RPC session setup. - Documents the rationale inline, noting the interaction with
retryTransientErrors: falseand supervisor-owned reconnection.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| RpcClient.makeProtocolSocket({ | ||
| retryTransientErrors: false, | ||
| retryPolicy: Schedule.recurs(0), | ||
| // With no protocol-level retry, a single missed pong escalates to a | ||
| // full session teardown, so the default 5s/5s heartbeat drops the | ||
| // connection whenever the server stalls past 5 seconds. Ping often | ||
| // enough to keep NAT/firewall state alive, but give the server room | ||
| // to stall before declaring the link dead. | ||
| pingInterval: "10 seconds", | ||
| pingTimeout: "20 seconds", | ||
| }), |
There was a problem hiding this comment.
Good catch — confirmed. effect@4.0.0-beta.78 (and current upstream effect-smol main) does not accept these options, so as originally pushed they would only work against a locally modified install.
Fixed in 986a153 by extending the repo's existing effect patch (patches/effect@4.0.0-beta.78.patch, which already customizes this same pinger for connection hooks): makePinger now takes a configurable ping interval and pong deadline (with a per-ping generation guard), makeProtocolSocket/layerProtocolSocket accept pingInterval/pingTimeout, and the .d.ts is updated accordingly. Defaults remain 5s/5s, so nothing changes for other callers.
Verified against the reinstalled patched package (not the local drift): a raw ws test server observed 10 pings in 11s with pingInterval: "1 second" (vs 2 with defaults), and onPingTimeout fired 4.0s after the last pong with 1s interval + 3s deadline (vs 10.0s with defaults). Full vp run typecheck and the client-runtime suite (260 tests) pass.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 28d0195. Configure here.
ApprovabilityVerdict: Needs human review This PR modifies RPC connection heartbeat timing which affects runtime behavior. An open review comment raises concerns that the new pingInterval/pingTimeout options may not be supported by the current Effect version and could be silently ignored, potentially making the fix ineffective. You can customize Macroscope's approvability policy. Learn more. |
Review bots correctly flagged that effect@4.0.0-beta.78 does not accept pingInterval/pingTimeout options, so passing them from session.ts alone would be ignored on a fresh install. Extend the repo's existing effect patch (which already customizes this pinger for connection hooks) so makeProtocolSocket accepts both options, defaulting to the current 5s/5s behavior when unset. Verified against the reinstalled patched package: a ws test server observed 10 pings in 11s with pingInterval "1 second" (vs 2 by default), and onPingTimeout fired 4.0s after the last pong with pingInterval 1s + pingTimeout 3s (vs 10.0s with defaults). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Fixes #3746
Symptom
Since 0.0.28, connections to a remote server constantly drop and reset. Users see the "connecting…" overlay appear mid-work (blocking the composer), often followed by "… did not respond during connection setup." On 0.0.27 the same setups are stable. It affects both direct LAN connections and SSH-tunneled ones, and it's worst when the host machine is under load (in my case a Windows 11 host; the mechanism itself is platform-independent, but hosts that stall longer — AV scanning, slower process spawning, heavy agent turns — hit it far more often, which may be why it's less visible in local Mac development).
Root cause
I bisected the 0.0.27 → 0.0.28 range. The behavior change comes from the connection architecture rewrite in #2978, via an interaction with effect's built-in socket heartbeat:
RpcClient.makeProtocolSockethas a built-in heartbeat with defaults of ping every 5s, pong deadline 5s. If a pong doesn't arrive within 5 seconds, the socket is failed withSocketError("ping timeout"). Neither the old nor the new code overrides these defaults.wsRpcProtocol.tscreated the protocol withretryTransientErrors: trueand an exponential retry policy, so a ping timeout was healed inside the protocol layer: the socket silently reconnected and in-flight requests were retried. Users never saw a 5-second stall.rpc/session.tscreates the protocol withretryTransientErrors: false, retryPolicy: Schedule.recurs(0), and reconnection ownership moved (deliberately, and reasonably) to the connection supervisor. But the supervisor's unit of recovery is the whole session: any socket failure →onDisconnect→ full teardown → re-establish socket + initial sync, under the supervisor's 15sCONNECTION_ESTABLISHMENT_TIMEOUT, with the blocking "connecting…" UI.So the net effect of the rewrite is that a single pong arriving later than 5 seconds — previously invisible — now tears down and rebuilds the entire session. A busy server event loop (long agent turn, git operations, file watching) or a latency spike on the tunnel is enough. And if the host is still busy during re-establishment, the 15s setup deadline also fails, producing exactly the "did not respond during connection setup" error from #3746, and the cycle repeats — the constant "reset, reset, reset" users report.
The server side is not involved:
RpcServeronly answers pings, it never enforces a deadline on clients, so this is fully addressable client-side.The fix (smallest change I could find)
Two parts:
pingInterval: "10 seconds", pingTimeout: "20 seconds"tomakeProtocolSocketinpackages/client-runtime/src/rpc/session.ts.patches/effect@4.0.0-beta.78.patch— which already customizes this same pinger for connection hooks) somakeProtocolSocket/layerProtocolSocketactually accept those options.makePingergains a configurable ping interval and pong deadline with a per-ping generation guard; defaults stay 5s/5s so no other caller changes behavior. (The review bots correctly caught that stockeffect@4.0.0-beta.78doesn't support these options — see the resolved threads.)Rationale for the values:
I intentionally did not re-enable protocol-level retry (
retryTransientErrors) — supervisor-owned reconnection from #2978 seems like the intended design, and transparent socket retries underneath it would fight that. This change just stops the supervisor from being invoked for stalls that aren't actually failures.Testing
vp run typecheckand the fullclient-runtimesuite pass (37 files, 260 tests).wsserver:pingInterval: "1 second"produced 10 pings in 11s (vs 2 with defaults), and the pong deadline fired at interval+timeout (4.0s configured vs 10.0s default).If the team prefers different ping values or wants this configurable, happy to adjust — I kept the diff minimal on purpose.